Optimize DeployToGrafana - #2359
Conversation
|
👋 cedric-cordenier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
✅ API Diff Results -
|
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed goroutine loop-variable capture bug in parallelFor, and getAlertRules can return duplicate rules leading to failing double-deletes/updates.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves Observability.DeployToGrafana performance by (1) parallelizing alert-rule upserts with a bounded concurrency limit and (2) removing N+1 Grafana alert-rule fetches by downloading the full rule list once and filtering in-memory.
Changes:
- Add a generic
parallelForhelper (bounded concurrency viaerrgroup.SetLimit) and internal tests. - Fetch alert rules once per deploy (
GetAlertRules) and filter locally instead of calling per-scope helper endpoints repeatedly. - Upsert alert rules concurrently with a new
DeployOptions.Concurrency(defaulting to 8).
File summaries
| File | Description |
|---|---|
| observability-lib/grafana/parallel.go | Adds a bounded-concurrency helper used to fan out alert-rule writes. |
| observability-lib/grafana/parallel_internal_test.go | Unit tests for the new parallel helper. |
| observability-lib/grafana/dashboard.go | Uses bounded parallelism for alert writes; optimizes alert-rule discovery by fetching once and filtering. |
| observability-lib/grafana/dashboard_deploy_test.go | Integration-style test verifying single fetch + concurrent writes against a fake Grafana server. |
| observability-lib/go.mod | Adds golang.org/x/sync dependency for errgroup. |
| observability-lib/go.sum | Adds checksums for the new golang.org/x/sync dependency. |
Review details
- Files reviewed: 5/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3289dc7 to
c889ae4
Compare
Addresses two inefficiencies in the current implementation of DeployToGrafana: * Previously, each alert generated a call to UpdateAlertRule, serially. Now this happens in parallel according to the defined concurrency factor. * getAlertRules had N+1 calls to grafana to fetch the alert rules. The current implementation fetches all the alert rules up front and does in-memory filtering instead. * Add a DeployCache to memoize fetching of folders and alert rules. Both of these improvements have reduced the latency for the CRE observability dashboards by 50%.
c889ae4 to
0b7799b
Compare
Addresses three inefficiencies in the current implementation of DeployToGrafana:
Both of these improvements have reduced the latency for the CRE observability dashboards by 50%.